Task done! Array metodo sort#2466
Task done! Array metodo sort#2466tiagodearaujo13-lab wants to merge 3 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a core requirement of the task is not yet met: you implemented [].__proto__.sort2 in src/arrayMethodSort.js, but you never wire it up so that Array.prototype.sort actually uses this implementation. As a result, calls like arr.sort() will still use the built‑in sort, which means the custom behavior the task asks you to implement is not in effect. To fix this, after defining sort2 (e.g., near the bottom of the file), explicitly assign it to the standard method, for example: Array.prototype.sort = Array.prototype.sort2;.
There is also a smaller but still important correctness issue: const comparator = compareFunction || defaultComparator; treats any falsy value (including null or 0) as if no comparator was provided, which diverges from the intended behavior; instead, check that compareFunction is actually a function, e.g. const comparator = typeof compareFunction === 'function' ? compareFunction : defaultComparator; or throw a TypeError for non-callables if required by the spec. Your bubble sort implementation and default string comparator logic look good and satisfy the algorithmic part of the task. Once you wire sort2 to Array.prototype.sort and tighten the comparator handling, your solution should meet the task requirements well. Keep up the solid work—these are straightforward but important final adjustments.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| function applyCustomSort() { | ||
| [].__proto__.sort2 = function(compareFunction) { | ||
| // write code here | ||
| [].__proto__.sort2 = function (compareFunction) { |
There was a problem hiding this comment.
You define [].__proto__.sort2 here, but the task requires implementing Array.prototype.sort using the predefined sort2. After defining sort2, assign it to the standard sort so calls to arr.sort() use your implementation (for example, set Array.prototype.sort to the sort2 implementation).
src/arrayMethodSort.js
Outdated
| return 0; | ||
| }; | ||
|
|
||
| const comparator = compareFunction || defaultComparator; |
There was a problem hiding this comment.
const comparator = compareFunction || defaultComparator; treats any falsy value (like null) as absent. Consider verifying the comparator is a function: const comparator = typeof compareFunction === 'function' ? compareFunction : defaultComparator; for more robust behavior.
src/arrayMethodSort.js
Outdated
|
|
||
| const comparator = compareFunction || defaultComparator; | ||
|
|
||
| for (let i = 0; i < this.length - 1; i++) { |
There was a problem hiding this comment.
You used a bubble-sort nested-loop implementation which is allowed for this task. As an optional improvement (not required), consider a more efficient algorithm for larger arrays.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a correctness issue with how the compareFunction argument is handled in src/arrayMethodSort.js, which affects compliance with Array.prototype.sort behavior. Currently, the code uses if (compareFunction) to decide whether to use a custom comparator; this treats any falsy non-function (e.g. null, 0) as if no comparator was supplied, instead of either using a valid function or throwing a TypeError. You should change this logic so that:
- If
compareFunction === undefined, you fall back to the default string-based comparator, and - If
compareFunction !== undefinedandtypeof compareFunction !== 'function', you thrownew TypeError('compareFunction must be a function'), and - Only when
typeof compareFunction === 'function'do you call it as the comparator.
This change is required because passing invalid comparators currently yields incorrect behavior instead of an error, which breaks functional expectations of the sort API. As a minor note (not blocking): your bubble sort implementation is fine for this exercise, but you may want to add a short comment that it isO(n²)and that more efficient algorithms (like quicksort or mergesort) would be preferable for large arrays. Overall, your wiring ofsort2toArray.prototype.sortand your default string comparison are good; once the comparator validation is corrected, the solution will meet the task’s core requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const a = this[j]; | ||
| const b = this[j + 1]; | ||
|
|
||
| if (compareFunction) { |
There was a problem hiding this comment.
Using if (compareFunction) treats any falsy value (e.g. null, 0) as if no comparator was provided. Check that the comparator is actually callable instead. For example: if (compareFunction !== undefined && typeof compareFunction !== 'function') throw new TypeError('compareFunction must be a function'); const comparator = typeof compareFunction === 'function' ? compareFunction : (a, b) => String(a) > String(b) ? 1 : (String(a) < String(b) ? -1 : 0);
src/arrayMethodSort.js
Outdated
| for (let i = 0; i < this.length; i++) { | ||
| for (let j = 0; j < this.length - 1; j++) { |
There was a problem hiding this comment.
This uses a nested-loop bubble sort (the double loop starting here). That's acceptable for the exercise, but note it's O(n²). For production or large arrays prefer a faster algorithm (quicksort/mergesort) as a future improvement.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn, even though there are a couple of small issues to fix.
You did a solid job implementing a custom Array.prototype.sort, wiring it through [].__proto__.sort2 and then assigning it to sort, and using a bubble-sort loop that correctly returns the array. One important detail to fix is the error message: when a non-function comparator (that is not undefined) is passed, the requirement is to throw TypeError('compareFunction must be a function'), but your code currently throws a different message; updating the string exactly will ensure tests that check the message pass. Also, instead of if (compareFunction) before calling the comparator, prefer if (typeof compareFunction === 'function') so that only actual functions are treated as comparators and to avoid any confusion with falsy values like 0 or ''. As a minor improvement, you could add a short comment that this implementation uses bubble sort and is O(n²), just to document its performance characteristics for large arrays.
Overall, your structure and understanding of custom sorting are good—these tweaks will help you align more closely with the specification and make your code clearer and more robust.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| throw new TypeError( | ||
| 'The comparison function must be either a function or undefined', | ||
| ); |
There was a problem hiding this comment.
The thrown TypeError message text is currently: The comparison function must be either a function or undefined. Tests and the required behavior expect the exact message compareFunction must be a function when a non-function (but not undefined) comparator is passed. Change the message to compareFunction must be a function to match expected behavior.
| if (compareFunction) { | ||
| if (compareFunction(a, b) > 0) { | ||
| shouldSwap = true; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
You use if (compareFunction) to decide whether to call the custom comparator. Although non-function values are checked above, it's clearer and more correct to use if (typeof compareFunction === 'function') to make explicit that only functions are treated as comparators (this mirrors the spec and avoids subtle falsy-value issues).
| do { | ||
| swapped = false; | ||
|
|
There was a problem hiding this comment.
Optional suggestion: add a short comment that this implementation uses bubble sort (O(n²)) and that more efficient algorithms (e.g. quicksort/mergesort) are preferable for large arrays. This is informational only and not required by the task.
No description provided.